Skip to content

[libc][test] make str_to_float_comparison_test independent of C++ headers. #133978

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

bassiounix
Copy link
Contributor

@bassiounix bassiounix commented Apr 1, 2025

This is an attempt to move away from C++ headers to be able to run the test without libcxx.
closes #129838

cc @lntue @RossComputerGuy

@bassiounix bassiounix changed the title [libc][test] make test independent of C++ headers [libc][test] make str_to_float_comparison_test independent of C++ headers. Apr 1, 2025
Copy link

github-actions bot commented Apr 1, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@bassiounix bassiounix force-pushed the str_to_float_comparison_test/hermitic_test branch from 387120c to b645618 Compare April 1, 2025 20:23
@lntue lntue requested review from michaelrj-google and lntue April 1, 2025 20:51
@llvmbot llvmbot added the libc label Apr 1, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 1, 2025

@llvm/pr-subscribers-libc

Author: Muhammad Bassiouni (bassiounix)

Changes

This is an attempt to move away from C++ headers to be able to run the test without libcxx.
closes #129838

cc @lntue @RossComputerGuy


Full diff: https://github.com/llvm/llvm-project/pull/133978.diff

2 Files Affected:

  • (modified) libc/test/src/__support/CMakeLists.txt (+14-23)
  • (modified) libc/test/src/__support/str_to_float_comparison_test.cpp (+46-48)
diff --git a/libc/test/src/__support/CMakeLists.txt b/libc/test/src/__support/CMakeLists.txt
index d056969034d69..6513e534607b9 100644
--- a/libc/test/src/__support/CMakeLists.txt
+++ b/libc/test/src/__support/CMakeLists.txt
@@ -249,31 +249,22 @@ add_libc_test(
     libc.src.__support.memory_size
 )
 
-# FIXME: We shouldn't have regular executables created because we could be
-#        cross-compiling the tests and running through an emulator.
 if(NOT LIBC_TARGET_OS_IS_GPU)
-  add_executable(
-    libc_str_to_float_comparison_test
-    str_to_float_comparison_test.cpp
-  )
-
-  target_link_libraries(libc_str_to_float_comparison_test
-    PRIVATE
-      "${LIBC_TARGET}"
-  )
-
-  add_executable(
-    libc_system_str_to_float_comparison_test
-    str_to_float_comparison_test.cpp
+  add_libc_test(
+    str_to_float_comparison_test
+    SUITE
+      libc-support-tests
+    SRCS
+      str_to_float_comparison_test.cpp
+    DEPENDS
+      libc.src.stdio.printf
+      libc.src.stdio.fopen
+      libc.src.stdio.fclose
+      libc.src.stdio.fgets
+      libc.src.stdlib.strtof
+      libc.src.stdlib.strtod
+    NO_RUN_POSTBUILD
   )
-
-  set(float_test_file ${CMAKE_CURRENT_SOURCE_DIR}/str_to_float_comparison_data.txt)
-
-  add_custom_command(TARGET libc_str_to_float_comparison_test
-                     POST_BUILD
-                     COMMAND ${CMAKE_CROSSCOMPILING_EMULATOR} $<TARGET_FILE:libc_str_to_float_comparison_test> ${float_test_file}
-                     COMMENT "Test the strtof and strtod implementations against precomputed results."
-                     VERBATIM)
 endif()
 
 add_subdirectory(CPP)
diff --git a/libc/test/src/__support/str_to_float_comparison_test.cpp b/libc/test/src/__support/str_to_float_comparison_test.cpp
index 61bfc3cd0903a..5f3100d3b5946 100644
--- a/libc/test/src/__support/str_to_float_comparison_test.cpp
+++ b/libc/test/src/__support/str_to_float_comparison_test.cpp
@@ -6,16 +6,14 @@
 //
 //===----------------------------------------------------------------------===//
 
-// #include "src/__support/str_float_conv_utils.h"
-
-#include <stdlib.h> // For string to float functions
-
-// #include "src/__support/FPUtil/FPBits.h"
-
-#include <cstdint>
-#include <fstream>
-#include <iostream>
-#include <string>
+#include "src/stdio/fclose.h"
+#include "src/stdio/fgets.h"
+#include "src/stdio/fopen.h"
+#include "src/stdio/printf.h"
+#include "src/stdlib/strtod.h"
+#include "src/stdlib/strtof.h"
+#include "test/UnitTest/Test.h"
+#include <stdint.h>
 
 // The intent of this test is to read in files in the format used in this test
 // dataset: https://github.com/nigeltao/parse-number-fxx-test-data
@@ -59,16 +57,18 @@ int checkFile(char *inputFileName, int *totalFails, int *totalBitDiffs,
   int32_t curFails = 0;    // Only counts actual failures, not bitdiffs.
   int32_t curBitDiffs = 0; // A bitdiff is when the expected result and actual
                            // result are off by +/- 1 bit.
-  std::string line;
-  std::string num;
+  char *line = nullptr;
+  char *num = nullptr;
 
-  std::ifstream fileStream(inputFileName, std::ifstream::in);
+  auto *fileHandle = LIBC_NAMESPACE::fopen(inputFileName, "r");
 
-  if (!fileStream.is_open()) {
-    std::cout << "file '" << inputFileName << "' failed to open. Exiting.\n";
+  if (!fileHandle) {
+    LIBC_NAMESPACE::printf("file '%s' failed to open. Exiting.\n",
+                           inputFileName);
     return 1;
   }
-  while (getline(fileStream, line)) {
+
+  while (LIBC_NAMESPACE::fgets(line, 100, fileHandle)) {
     if (line[0] == '#') {
       continue;
     }
@@ -76,13 +76,13 @@ int checkFile(char *inputFileName, int *totalFails, int *totalBitDiffs,
     uint32_t expectedFloatRaw;
     uint64_t expectedDoubleRaw;
 
-    expectedFloatRaw = fastHexToU32(line.c_str() + 5);
-    expectedDoubleRaw = fastHexToU64(line.c_str() + 14);
-    num = line.substr(31);
+    expectedFloatRaw = fastHexToU32(line + 5);
+    expectedDoubleRaw = fastHexToU64(line + 14);
+    num = line + 31;
 
-    float floatResult = strtof(num.c_str(), nullptr);
+    float floatResult = LIBC_NAMESPACE::strtof(num, nullptr);
 
-    double doubleResult = strtod(num.c_str(), nullptr);
+    double doubleResult = LIBC_NAMESPACE::strtod(num, nullptr);
 
     uint32_t floatRaw = *(uint32_t *)(&floatResult);
 
@@ -101,9 +101,8 @@ int checkFile(char *inputFileName, int *totalFails, int *totalBitDiffs,
         curFails++;
       }
       if (curFails + curBitDiffs < 10) {
-        std::cout << "Float fail for '" << num << "'. Expected " << std::hex
-                  << expectedFloatRaw << " but got " << floatRaw << "\n"
-                  << std::dec;
+        LIBC_NAMESPACE::printf("Float fail for '%s'. Expected %x but got %x\n",
+                               num, expectedFloatRaw, floatRaw);
       }
     }
 
@@ -120,14 +119,14 @@ int checkFile(char *inputFileName, int *totalFails, int *totalBitDiffs,
         curFails++;
       }
       if (curFails + curBitDiffs < 10) {
-        std::cout << "Double fail for '" << num << "'. Expected " << std::hex
-                  << expectedDoubleRaw << " but got " << doubleRaw << "\n"
-                  << std::dec;
+        LIBC_NAMESPACE::printf(
+            "Double fail for '%s'. Expected %lx but got %lx\n", num,
+            expectedDoubleRaw, doubleRaw);
       }
     }
   }
 
-  fileStream.close();
+  LIBC_NAMESPACE::fclose(fileHandle);
 
   *totalBitDiffs += curBitDiffs;
   *totalFails += curFails;
@@ -138,7 +137,7 @@ int checkFile(char *inputFileName, int *totalFails, int *totalBitDiffs,
   return 0;
 }
 
-int main(int argc, char *argv[]) {
+TEST(LlvmLibcStrToFloatComparisonTest, CheckFile) {
   int result = 0;
   int fails = 0;
 
@@ -150,24 +149,23 @@ int main(int argc, char *argv[]) {
   int detailedBitDiffs[4] = {0, 0, 0, 0};
 
   int total = 0;
-  for (int i = 1; i < argc; i++) {
-    std::cout << "Starting file " << argv[i] << "\n";
-    int curResult =
-        checkFile(argv[i], &fails, &bitdiffs, detailedBitDiffs, &total);
-    if (curResult == 1) {
-      result = 1;
-      break;
-    } else if (curResult == 2) {
-      result = 2;
-    }
+
+  char filename[] = "str_to_float_comparison_data.txt";
+  LIBC_NAMESPACE::printf("Starting file %s\n", filename);
+  int curResult =
+      checkFile(filename, &fails, &bitdiffs, detailedBitDiffs, &total);
+  if (curResult == 1) {
+    result = 1;
+  } else if (curResult == 2) {
+    result = 2;
   }
-  std::cout << "Results:\n"
-            << "Total significant failed conversions: " << fails << "\n"
-            << "Total conversions off by +/- 1 bit: " << bitdiffs << "\n"
-            << "\t" << detailedBitDiffs[0] << "\tfloat low\n"
-            << "\t" << detailedBitDiffs[1] << "\tfloat high\n"
-            << "\t" << detailedBitDiffs[2] << "\tdouble low\n"
-            << "\t" << detailedBitDiffs[3] << "\tdouble high\n"
-            << "Total lines: " << total << "\n";
-  return result;
+
+  EXPECT_EQ(result, 0);
+  EXPECT_EQ(fails, 0);
+  EXPECT_EQ(bitdiffs, 0);
+  EXPECT_EQ(detailedBitDiffs[0], 0);
+  EXPECT_EQ(detailedBitDiffs[1], 0);
+  EXPECT_EQ(detailedBitDiffs[2], 0);
+  EXPECT_EQ(detailedBitDiffs[3], 0);
+  EXPECT_EQ(total, 0);
 }

Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM with some minor comments. Thanks for cleaning up this old test!

@bassiounix bassiounix force-pushed the str_to_float_comparison_test/hermitic_test branch from 37b1331 to a0d1cff Compare April 2, 2025 21:20
@bassiounix bassiounix requested a review from lntue April 2, 2025 21:24
@bassiounix bassiounix requested a review from lntue April 5, 2025 02:54

fileStream.close();
int checkFile(char *inputFileName, int *totalFails, int *totalBitDiffs,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can get rid of this since the data will be available on the test now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikhailramalho @RossComputerGuy I think the purpose of this test according to @michaelrj-google is to manually run it against large files containing many interesting cases. Before this PR, the test build was set up manually, and didn't respect the dependency or entrypoint lists that we normally have. With this change, this test will be build using our normal testing infra as other tests, respecting the dependency (skipped if they are not available), and will not automatically run with ninja check-libc. I hope that would be enough to make it non-blocking when we try to build on new targets?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, but if the goal is to run it manually on large files, can we remove the file from the repo and keep the static data for the automated test?

If so, you can ignore my comments to remove the file handling stuff @bassiounix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing the file sounds good to me, but we should add more comments about the usage of this test, and maybe add the link to the other repo with a lot of tests for this one. I'll let @michaelrj-google decide what to do with the test file(s).

@bassiounix bassiounix force-pushed the str_to_float_comparison_test/hermitic_test branch from d966884 to c90a4da Compare April 7, 2025 05:34
Copy link
Contributor

@lntue lntue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's wait for @michaelrj-google @mikhailramalho and @RossComputerGuy to double check.

@lntue lntue requested a review from RossComputerGuy April 8, 2025 20:27
Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@mikhailramalho mikhailramalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nits, otherwise it's okay.

Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On local testing I found a few issues, specifically it doesn't seem to work with files. Will provide more information after some testing.

Copy link
Member

@mikhailramalho mikhailramalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Please wait for Michael's approval before landing this.

@RossComputerGuy
Copy link
Member

There's CI failures, I'll be able to look at this soon.

Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM with some minor style nits. The test works locally.

Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for getting this cleaned up!

@michaelrj-google michaelrj-google merged commit 785d69e into llvm:main Apr 9, 2025
14 of 15 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Apr 9, 2025

LLVM Buildbot has detected a new failure on builder libc-riscv32-qemu-yocto-fullbuild-dbg running on rv32gc-qemu-system while building libc at step 4 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/196/builds/6823

Here is the relevant piece of the build log for the reference
Step 4 (annotate) failure: 'python ../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py ...' (failure)
...
@@@BUILD_STEP build libc-startup@@@
Running: ninja libc-startup
ninja: no work to do.
@@@BUILD_STEP libc-unit-tests@@@
Running: ninja libc-unit-tests
[1/980] Building CXX object libc/src/stdlib/CMakeFiles/libc.src.stdlib.getenv.__internal__.dir/getenv.cpp.o
[2/980] Building CXX object libc/test/src/__support/CMakeFiles/libc.test.src.__support.str_to_float_comparison_test.__unit__.dir/str_to_float_comparison_test.cpp.o
[3/980] Building CXX object libc/src/stdio/generic/CMakeFiles/libc.src.stdio.generic.printf.__internal__.dir/printf.cpp.o
[4/980] Linking CXX executable libc/test/src/__support/libc.test.src.__support.str_to_float_comparison_test.__unit__
[5/980] Running unit test libc.test.src.__support.freelist_test.__unit__
FAILED: libc/test/src/__support/CMakeFiles/libc.test.src.__support.freelist_test.__unit__ /home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support/CMakeFiles/libc.test.src.__support.freelist_test.__unit__ 
cd /home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support && /home/libcrv32buildbot/cross.sh libc.test.src.__support.freelist_test.__unit__.__build__
rsync: [sender] change_dir "/home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support/libc.test.src.__support.freelist_test.__unit__.__build__/build" failed: Not a directory (20)
rsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1338) [sender=3.3.0]
sh: line 1: /timer.21436: Permission denied
sh: line 1: /home/libcrv32buildbot/tests/: Is a directory
[6/980] Running unit test libc.test.src.__support.fixedvector_test.__unit__
FAILED: libc/test/src/__support/CMakeFiles/libc.test.src.__support.fixedvector_test.__unit__ /home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support/CMakeFiles/libc.test.src.__support.fixedvector_test.__unit__ 
cd /home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support && /home/libcrv32buildbot/cross.sh libc.test.src.__support.fixedvector_test.__unit__.__build__
rsync: [sender] change_dir "/home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support/libc.test.src.__support.fixedvector_test.__unit__.__build__/build" failed: Not a directory (20)
rsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1338) [sender=3.3.0]
sh: line 1: /timer.21440: Permission denied
sh: line 1: /home/libcrv32buildbot/tests/: Is a directory
[7/980] Running unit test libc.test.src.__support.blockstore_test.__unit__
FAILED: libc/test/src/__support/CMakeFiles/libc.test.src.__support.blockstore_test.__unit__ /home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support/CMakeFiles/libc.test.src.__support.blockstore_test.__unit__ 
cd /home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support && /home/libcrv32buildbot/cross.sh libc.test.src.__support.blockstore_test.__unit__.__build__
rsync: [sender] change_dir "/home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support/libc.test.src.__support.blockstore_test.__unit__.__build__/build" failed: Not a directory (20)
rsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1338) [sender=3.3.0]
sh: line 1: /timer.21442: Permission denied
sh: line 1: /home/libcrv32buildbot/tests/: Is a directory
[8/980] Running unit test libc.test.src.__support.endian_internal_test.__unit__
FAILED: libc/test/src/__support/CMakeFiles/libc.test.src.__support.endian_internal_test.__unit__ /home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support/CMakeFiles/libc.test.src.__support.endian_internal_test.__unit__ 
cd /home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support && /home/libcrv32buildbot/cross.sh libc.test.src.__support.endian_internal_test.__unit__.__build__
rsync: [sender] change_dir "/home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support/libc.test.src.__support.endian_internal_test.__unit__.__build__/build" failed: Not a directory (20)
rsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1338) [sender=3.3.0]
sh: line 1: /timer.21438: Permission denied
sh: line 1: /home/libcrv32buildbot/tests/: Is a directory
[9/980] Running unit test libc.test.src.__support.freestore_test.__unit__
FAILED: libc/test/src/__support/CMakeFiles/libc.test.src.__support.freestore_test.__unit__ /home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support/CMakeFiles/libc.test.src.__support.freestore_test.__unit__ 
cd /home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support && /home/libcrv32buildbot/cross.sh libc.test.src.__support.freestore_test.__unit__.__build__
rsync: [sender] change_dir "/home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support/libc.test.src.__support.freestore_test.__unit__.__build__/build" failed: Not a directory (20)
rsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1338) [sender=3.3.0]
sh: line 1: /timer.21451: Permission denied
sh: line 1: /home/libcrv32buildbot/tests/: Is a directory
[10/980] Running unit test libc.test.src.__support.str_to_integer_test.__unit__
FAILED: libc/test/src/__support/CMakeFiles/libc.test.src.__support.str_to_integer_test.__unit__ /home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support/CMakeFiles/libc.test.src.__support.str_to_integer_test.__unit__ 
cd /home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support && /home/libcrv32buildbot/cross.sh libc.test.src.__support.str_to_integer_test.__unit__.__build__
rsync: [sender] change_dir "/home/libcrv32buildbot/bbroot/libc-riscv32-qemu-yocto-fullbuild-dbg/build/libc/test/src/__support/libc.test.src.__support.str_to_integer_test.__unit__.__build__/build" failed: Not a directory (20)
rsync error: some files/attrs were not transferred (see previous errors) (code 23) at main.c(1338) [sender=3.3.0]

AllinLeeYL pushed a commit to AllinLeeYL/llvm-project that referenced this pull request Apr 10, 2025
…eaders. (llvm#133978)

This is an attempt to move away from C++ headers to be able to run the
test without `libcxx`.
closes llvm#129838

cc @lntue @RossComputerGuy
@bassiounix bassiounix deleted the str_to_float_comparison_test/hermitic_test branch April 10, 2025 07:18
var-const pushed a commit to ldionne/llvm-project that referenced this pull request Apr 17, 2025
…eaders. (llvm#133978)

This is an attempt to move away from C++ headers to be able to run the
test without `libcxx`.
closes llvm#129838

cc @lntue @RossComputerGuy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[libc] str_to_float_comparison_test should be hermetic
7 participants